Interesting, thank you!

Perhaps instead of modifying TableRenderers it might be best to leave that file alone and create a new component SubtotalTableRenderers beside it, and then surgically see what can be pulled out as common components? as it stands right now the diff is hard to read :)

The same might actually apply to PivotData ... instead of parameterizing it, perhaps it would make sense to have a subtotal-aware copy of it? that's less clear, and is one of the reasons why Subtotal.js is apart from the main PivotTable.js library.

Nicolas,

Totally understood. This is a pretty large PR. And apologies for taking so long to get back to you. Work just kept getting in the way.

For better understanding the changes:

• Commit dbc8168 is fairly minor and adds in the rowTotal and colTotal options.
• Commits f067c1d through f097b1c refactor the table renderers to make them a little easier to understand and modify. They don't change the functionality and mostly just move code from the primary render method to separate methods within the same class.
• Commit 391be64 is where the bulk of the subtotal changes are. Hopefully they should be much easier to understand vs. commit f097b1c since I kept the same basic code layout.
• Commit c9a2067 refactors the PivotTableUI just a little bit to make it easier to customize (I use this outside the react-pivottable package). This commit is pretty independent from the rest.
• The rest of the changes are minor cleanups, updating for babel presets that I hadn't realized had been a problem, etc...

Regarding splitting out the subtotal code:

• I thought about that when writing this. I ultimately settled on keeping them in the same class because there is quite a bit of shared code between the two.
• In PivotData:
  • The only meaningful change is in processRecord where we loop through all prefixes of the row and column keys instead of just the entire thing.
  • There are a handful of minor changes to make shorter key arrays sort either before or after the longer arrays (based on user settings).
• In TableRenderers:
  • the span calculation needs to be slightly modified to account for shorter keys.
  • I pre-compute the callbacks because that speeds things up a little bit (but it's not really necessary).
  • I had to add in a little more argument parsing.
  • The biggest changes are for the expand/collapse functionality.
    • I added in a filtering step to remove collapsed (and therefore hidden) keys.
    • I needed a little bit of extra logic to add in spacing cells for keys corresponding to subtotals.
    • And I had to add in some logic to populate the arrow characters with the associated click handlers.
  • BUT: the underlying code structure and methods used are still pretty much identical to the non-subtotal case.

Hope this helps!

OK, thanks for the detailed response! I promise to take a close look in the coming week: I'm just buckling down for a big release at the moment.

One thing which would make both reviewing and merging easier is the addition of a test or two for the modifications to PivotData ;)

That I can do.

Also if it'll help, I can re-arrange the commits into something a bit more logical. Feel free to ask.

Your description of the commits is great, no need to rearrange anything! A test would help and then it's a question of me finding the time to QA/review :)

FYI: I added in that test for PivotData and the prepare script. Just wondering if you had any other requests.

Thanks.

Hi,

Just wondering if you'd gotten a chance to take a look at this.

Thanks.

Hello again Nicolas,

Got a chance yet? Any way I can help?

Thanks.

I'm looking at this today. Apologies on the delay.

OK so I cloned this and ran yarn update && yarn start to run the demo app, but the Table renderer throws React errors. Are you seeing this also? Can you fix this please?

In terms of getting this merged in, this really is too big a PR to easily review... Do you think you could submit a series of smaller ones or something? Maybe one for the row/total columns? And a separate one that just does some of the cleanup/refactoring without changing the behaviour? all in the service of making the bigger behaviour-change PR easier to review?

Sorry, I know it's a big ask after a long silence, but I would like to get this sorted and release your hard work!

I'll take a look at the errors and split up the pr. Thanks for taking a look.

Did you still intend to look at the errors and/or break up this PR into more-digestible chunks?

Yes. Eventually... Work has just gotten busy.

(And yes, I recognize the irony in pushing for a review and then holding up progress myself. My sincere apologies.)

No worries ;)

Any progress on this - I'm presuming the PR is a step forward for the module... is it best to spend more time and break it up into pieces, or accept it as is?
I would like to add a module for Vega-lite renderers. It doesn't look too hard to do. My quandary is which branch to use to base my work on. I am happy to help if needed

I recommend adding to the current master. I don't know when I'll get around to updating this PR and besides the changes are pretty self contained in the table renderers.

Bump. Willing to sponsor project updates.

@thetwosents please email me at nicolas@plotly.com and we can chat about sponsorship?